-
-
Notifications
You must be signed in to change notification settings - Fork 842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Squash core migrations #2842
Squash core migrations #2842
Conversation
Data migrations (seed default groups, seed default permissions) are deliberately excluded. This also allows us to remove a lot of now unnecessary public API from the migrator and migration repository.
@@ -46,6 +46,8 @@ public function register() | |||
ResetCommand::class, | |||
ScheduleListCommand::class, | |||
ScheduleRunCommand::class | |||
// Used internally to create DB dumps before major releases. | |||
// \Flarum\Database\Console\GenerateDumpCommand::class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to put this command inside of the source code?
We likely won't need it before a while, and it might end up broken without us even realizing it in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is why I commented it out. I wanted to keep it so that we could debug how the dump was created if we run into issues with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't really like the idea of having that code lying around unused for 1,2,5 years? in the repo. I feel like it would be best inside of an issue, or a dedicated branch so we can find it again later but don't ship it / maintain it as part of the whole 1.x release cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to put it in the Flarum CLI, but didn't have time to rewrite it so that it worked for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably good like is is now. If there's any issue with the approach QA should hopefully find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at this previously and it works well locally.
[ci skip] [skip ci]
Fixes #2258
Changes proposed in this pull request:
Unfortunately we can't do extension migrations here, as we have no way of filtering out data migrations other than hardcoding in a list. Their impact is relatively small though, so it shouldn't be that bad.
Confirmed
composer test
).